Earth - Maha & Sophie - Slack API #22

steve-messing · 2020-10-10T00:07:17Z

Assignment Submission: Slack CLI

Congratulations! You're submitting your assignment. Please reflect on the assignment with these questions.

Reflection

Question	Answer
How did you go about exploring the Slack API? Did you learn anything that would be useful for your next project involving an API?	Slack's documentation was helpful for conversations.list and users.list.
Give a short summary of the request/response cycle. Where does your program fit into that scheme?	Send a request to an API and get a response in the form of JSON. Our Recipient class handles the request/response cycle when it calls the Slack API.
How does your program check for and handle errors when using the Slack API?	Raise SlackApiError
How did the design and organization of your project change over time?	Notably we started by calling the API directly from slack.rb, and as the program developed and we built understanding, we moved the API calls further out, into User and Class, and eventually to Recipient.
Did you use any of the inheritance idioms we've talked about in class? How?	User < Recipient and Class < Recipient are both examples of inheritance idioms.
How does VCR aid in testing a program that uses an API?	Stores the result of an API call in a file rather than calling the API over and over.

tildeee

Slack CLI

Major Learning Goals/Code Review

Criteria	yes/no, and optionally any details/lines of code to reference
Practices best practices working with APIs. The `.env` is not checked into git, and no API token was directly used in the Ruby code without `ENV`.	✔️
Practices error handling with APIs. For all pieces of code that make an API call, it handles API requests that come back with errors/error status codes appropriately.	Mostly -- It might be good to check in `Channel#list_all` and `User#list_all` to have behavior in case the `self.get` returns unsuccessfully. Your code actually does break if `self.get` returned unsuccessfully! It would also be nice to start anticipating failures like this in the tests for those methods, too!
Implements inheritance and inheritance idioms. There is a `Recipient` class. `User` and `Channel` inherit from `Recipient`. In `Recipient`, there are appropriate methods defined that are used in both `User` and `Channel`. Some may be implemented. Some may be template methods.	✔️
Practices clean code. `lib/slack.rb` only interacts with `Workspace` to show a separation of responsibilities. Complex logic is broken into smaller helper methods.	✔️
Practices instance methods vs. class methods appropriately. The methods to list all Channels or Users is likely a class method within those respective classes.	✔️
Practices best practices for testing. The project has and uses VCR mocking when running tests, and can run offline.	✔️, Yes! Wonderful! However, not all of your VCR casette files are checked into git and I would expect those to be included in your project
Practices writing tests. The `User`, `Channel`, and `Workspace` classes have unit tests.	✔️, YES!!!!!
Practices writing tests. There are tests for sending messages (the location of these tests may differ, but is likely in `Recipient`)	✔️, YES!!!!!!
Practices git with at least 15 small commits and meaningful commit messages	There weren't that many commits, but more importantly, commit messages should describe the changes in the commit. They should not be something like "Wave 2 complete," or "changed tests." Try things like, "implements the list_all method in Channel" or "updates the Workspace tests to use helper methods," or "refactors the main script to use a case statement"

Functional Requirements

Functional Requirement	yes/no
As a user of the CLI program, I can list users and channels	✔️
As a user of the CLI program, I can select users and channels	✔️
As a user of the CLI program, I can show the details of a selected user or channel	✔️
As a user of the CLI program, when I input something inappropriately, the program runs without crashing	✔️

Overall Feedback

Overall Feedback	Criteria	yes/no
Green (Meets/Exceeds Standards)	7+ in Code Review && 3+ in Functional Requirements	✔️
Yellow (Approaches Standards)	6+ in Code Review && 2+ in Functional Requirements
Red (Not at Standard)	0-5 in Code Review or 0,1 in Functional Reqs, or assignment is breaking/doesn’t run with less than 5 minutes of debugging

Additional Feedback

AHHH! AMAZING! Hi Maha and Sophie! Your project submission is great!! The code looks good, it works as intended, and your tests are exactly the level of detail that I hope for in a software engineer intern. You all did a great job on this project!!

Seriously, the clear logic and patterns that you all set in your tests and the way you covered all of the edge cases is great, keep it up.

Also, from how Workspace and slack.rb are set up, I can tell that you all put extra effort into refactoring it and making the logic and also user experience nice.

I've added just a few comments about small things to consider. Let me know if any questions pop up.

Well done! Keep up the good work!

Code Style Bonus Awards

Was the code particularly impressive in code style for any of these reasons (or more...?)

Quality	Yes?
Perfect Indentation	✅
Elegant/Clever
Descriptive/Readable	✅
Concise	✅
Logical/Organized	✅

tildeee · 2020-10-23T17:02:37Z

lib/slack.rb

+  begin
  workspace = Workspace.new
+  rescue SlackApiError => error
+    puts "error occurred: #{error.message}"
+    return
+  end


this is awesome, great thinking!

tildeee · 2020-10-23T17:03:33Z

lib/user.rb

+  end
+
+  def self.list_all
+    user_url = 'https://slack.com/api/users.list'


potential refactoring: because we never expect to re-assign user_url, it might be worth making this into a constant variable, USER_URL

tildeee · 2020-10-23T17:04:20Z

lib/workspace.rb

+  end
+
+  def select_channel(input)
+    @selected = @channels.find { |channel| input == channel.name || input == channel.slack_id}


Great work refactoring this find to include the ||. This method looks great.

tildeee · 2020-10-23T17:05:42Z

lib/workspace.rb

+
+  def ask_to_select(input)
+    select_channel(input)
+    if @selected == nil


For fun, just as an FYI: Ruby has a method .nil? you can attach to most objects, so here you could do if @selected.nil? .

nil? returns true only if the attached object is nil.

tildeee · 2020-10-23T17:08:24Z

test/channel_test.rb

+    it "returns an error for an invalid token" do
+      VCR.use_cassette("channel_send_not_authed") do
+        channel = Channel.new("C01BTVCEXCN", "general", "topic", 3)
+
+        expect { channel.send_message("This post should not work") }.must_raise SlackApiError
+      end
+    end


This is an incredibly awesome and thoughtful test to include. The fact that y'all thought about this shows me that you've put a lot of thought into this material; this is awesome!

On my machine, I'm unable to get this test to pass. For me, I think that this is completely expected-- it's actually really tough to test that our API call is using an invalid token based on how we implemented it! If we look over on line 23-24 for a successful send_message, we can see that the test looks pretty identical to the code in this test!

In general, that's because this concept is hard to test-- it's not very testable. This is my way of saying, "to test this correctly, we'd probably have to put in a LOT of extra work... that's outside the scope of this project."

However, if y'all got this test to pass, let me know!! :D I want to learn.

tildeee · 2020-10-23T17:08:56Z

test/workspace_test.rb

+    end
+  end
+
+  describe "can select users or channels" do


Your tests here are exactly what I was looking for. Keep this up!

tildeee · 2020-10-23T17:09:31Z

test/workspace_test.rb

+  describe "send_message" do
+    before do
+      VCR.use_cassette("send_message") do
+        @workspace = Workspace.new
+      end
+    end
+
+    it "can send a message" do
+      VCR.use_cassette("send_message") do
+        @workspace.select_user("sophie.e.messing")
+        @workspace.send_message("This post should work")
+      end
+    end
+
+    it "will raise an error if we send a message with no channel or user" do
+      expect { @workspace.send_message("This post should not work") }.must_raise SlackApiError
+    end
+  end


Perfect tests all over this project, I'm crying 😭

steve-messing added 7 commits October 6, 2020 11:32

Add coverage directory to .gitignore file

8f8b84e

setup complete

fbb425c

wave 1 complete

8559d46

wave 2 complete

c2347fa

wave 3 complete

610d568

better error handling for invalid entry in slack.rb

cc2cc72

wave 3 complete. test coverage 100%

6c3709e

tildeee reviewed Oct 23, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Earth - Maha & Sophie - Slack API #22

Earth - Maha & Sophie - Slack API #22

steve-messing commented Oct 10, 2020

tildeee left a comment

tildeee Oct 23, 2020

tildeee Oct 23, 2020

tildeee Oct 23, 2020

tildeee Oct 23, 2020

tildeee Oct 23, 2020

tildeee Oct 23, 2020

tildeee Oct 23, 2020

Earth - Maha & Sophie - Slack API #22

Are you sure you want to change the base?

Earth - Maha & Sophie - Slack API #22

Conversation

steve-messing commented Oct 10, 2020

Assignment Submission: Slack CLI

Reflection

tildeee left a comment

Choose a reason for hiding this comment

Slack CLI

Major Learning Goals/Code Review

Functional Requirements

Overall Feedback

Additional Feedback

Code Style Bonus Awards

tildeee Oct 23, 2020

Choose a reason for hiding this comment

tildeee Oct 23, 2020

Choose a reason for hiding this comment

tildeee Oct 23, 2020

Choose a reason for hiding this comment

tildeee Oct 23, 2020

Choose a reason for hiding this comment

tildeee Oct 23, 2020

Choose a reason for hiding this comment

tildeee Oct 23, 2020

Choose a reason for hiding this comment

tildeee Oct 23, 2020

Choose a reason for hiding this comment